-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow filtering workflows by more than one status value #41
Allow filtering workflows by more than one status value #41
Conversation
Hi, @fchimpan is there anything I can do to help you review/merge this PR? |
Hi, @IvanRibakov |
Hi @fchimpan, any thoughts on the PR? |
@@ -138,3 +125,43 @@ func workflowStats(cfg config, opt options, isJobs bool) error { | |||
|
|||
return nil | |||
} | |||
|
|||
func fetchWorkflowRuns(ctx context.Context, client *github.WorkflowStatsClient, cfg config, opt options) ([]*go_github.WorkflowRun, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought fetchWorkflowRuns
function could be called directly within the client.FetchWorkflowRuns
method rather than being defined as a separate function. Is there a specific reason why it was defined as a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason why it was defined as a function?
Mainly to avoid bloating workflowStats
function which is already fairly long. Feel free to adjust formatting to your liking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IvanRibakov
Sorry for the delayed review, and thank you for the amazing feature addition! I believe there are no issues with the content.
f2286ac
to
cba9515
Compare
@IvanRibakov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
This PR turns
--status
flag into a slice of strings, allowing to filter by more than one status value.Primary motivation for this change is to be able to exclude
cancelled
workflow runs from the success/failure analysis as we are usingcancel-in-progress: true
workflow setting which results in quite a big number of cancelled jobs that significantly skew reported success/failure rate:P.S.
This PR also fixes a bug where using previous status filter implementation did NOT correctly filter all run attempts by status as only the last attempts were filtered while previous run attempts were added to the result set without filtering by status.